Looking at the code, it might make sense (now we're going from 2 to 3) to split out the 'tryAsWindowsNamedPipe', 'tryAsUnixDomainSocket' and 'tryAsPrivateKeyFile' logic into three functions.

That would simplify the toAgent loop, and would also let us move the "If Windows, os.ModeSocket is wrong, so do something else" logic into a Windows-specific 'tryAsUnixDomainSocket' implementation, which could then use reparse-point tag checking to be sure about unix domain socket-ness, rather than hoping no one points you at a file that contains the word 'socket'.

PTAL linter errors in CI

Will fix.

split out the 'tryAsWindowsNamedPipe', 'tryAsUnixDomainSocket' and 'tryAsPrivateKeyFile' logic

So we'd have something like this?

if parsed := tryWindowsPipePath(); parsed != nil {
    socket = parsed
    continue
}

if parsed, err := tryUnixSocketPath(p); err != nil {
    return source{}, errors.WithStack(err)
} else if parsed != nil {
    socket = parsed
    continue
}

// open file
// read contents
// try parse private key
// (on failure check contains "socket") ?

I'm not convinced that the contains "socket" check should be kept. It turns out that Windows native UNIX sockets cannot actually be read from like a normal file. Rather, using os.Open() or os.Stat() on a socket fails immediately with an The file cannot be accessed by the system error. It seems that os.Stat() fails because it actually tries to open the file (on Windows).

The only Windows socket-y files that I can think of that have this kind of content are cygwin and/or msys sockets (ref). While I haven't explicitly tested this, I'm doubtful that Go even supports connecting to these "sockets" using net.Dial() or similar functions.

Thoughts?

move the "If Windows, os.ModeSocket is wrong, so do something else" logic into a Windows-specific 'tryAsUnixDomainSocket' implementation

Sounds reasonable.

use reparse-point tag checking to be sure about unix domain socket-ness

Wow, that just consumed most of my evening. TIL that AF_UNIX has been implemented on Windows. These sockets are represented on the filesystem by reparse points. However, due to a bug (ref) in Windows 1903+, these files are not reported as reparse points.

So, FindFirstFile does not set FILE_ATTRIBUTE_REPARSE_POINT and hence cannot be used to check for UNIX sockets. However, we can invoke CreateFile(OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT) to check whether the file is, in fact, a reparse point. If that call succeeds, then DeviceIoControl(FSCTL_GET_REPARSE_POINT) can be used to determine the ReparseTag to check for IO_REPARSE_TAG_AF_UNIX. If so, then the file is definitely a UNIX socket, even if the ModeSocket flag is absent.

So we'd have something like this?

Yeah, that was pretty-much what I was thinking.

As far as the contains "socket" test, I had been wondering what that was about, but just assumed it worked because it's there. I had assumed that was for AF_UNIX sockets (and Go's os module being daft about reparse points generally), but you're probably right, it's probably about cygwin emulating AF_UNIX sockets, so I can't see how they could work for things that aren't also running under cygwin (and hence having open intercepted to handle such things).

I guess the use-case here is people running a cygwin-based ssh-agent build.

Side-band, I find myself wondering now if cygwin supports AF_UNIX sockets natively when supported, i.e. Windows version 1803 and later, which would mean by 2022 the emulated socket files become a problem only on out-of-support OSes.

I have updated my branch with the requested changes. I also have a working implementation of native UNIX socket support on Windows. Would you prefer I make that part of this PR, or split it out into a separate one?

Also, any verdict on whether to remove the contains "socket" as part of implementing proper UNIX socket support? I'm fairly sure that that specific code path will never work. While a Cygwin build of ssh-agent might present such a "sockety" file on a Windows filesystem, dialing that path with net.Dial() will not work. If buildkit were built under Cygwin, the code path would never trigger because the os.Stat() result would have os.ModeSocket set.

For reference, contains "socket" was introduced in #1695 to fix #914, and points at golang/go#33357, which do indeed appear to be referring to Unix Domain Sockets under Windows, not cygwin. And it looks like it might have varied by OS build, but it's not clear how.

So I'd keep it as part of the Windows flow for now, and separately investigate it more carefully, if warranted.